Skip to content

fix: dj.Top is honored when a limited expression is used in another query - #1657

Merged
dimitri-yatsenko merged 1 commit into
masterfrom
fix/top-in-joins
Nov 10, 2026
Merged

dimitri-yatsenko merged 1 commit into
masterfrom
fix/top-in-joins

Conversation

@dimitri-yatsenko

@dimitri-yatsenko dimitri-yatsenko commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

Fixes #1656. Fixes #1666 (a duplicate of #1656).

The bug

An expression restricted by dj.Top keeps its ORDER BY/LIMIT in _top, separately from its WHERE conditions. Several operations that take an expression as an operand ignored _top, or broke on it. They returned too many rows without a warning, or failed with a SQL error. All of the following reproduce on master on MySQL 8.0 and PostgreSQL. top_child selects one child, and top_parent selects parent 1.

Query Before
Parent * (Child & top_child), (Parent & top_parent) * Child, both, or dj.Top together with a condition full join, limit dropped
Parent.aggr(Child & top_child, ..., exclude_nonmatching=True), (Parent & top_parent).aggr(Child, ...), (Child & top_child).extend(Parent) limit dropped
dj.U("color").aggr(Child & top_child, ...) limit dropped
Parent & (Child & top_child), Parent - (Child & top_child) MySQL: error 1235 (LIMIT & IN subquery); PostgreSQL: correct
dj.U("parent_id") & (Child & dj.Top(2, ...)) MySQL 3065 or PostgreSQL DISTINCT/ORDER BY error when the order is on another attribute; otherwise the limit is applied after DISTINCT
(A + B) & dj.Top(...) syntax error: the ORDER BY … LIMIT sat inside the quoted alias name
(Parent & top_parent) + (ParentInfo & dj.Top(...)) wrong rows
(Parent.aggr(Child, n="count(*)") & dj.Top(1, order_by="n DESC")).keys() Unknown column 'n' (proj() dropped the ordering attribute)
(Parent & dj.Top(1, order_by="label DESC")).proj(label="-parent_id") another row: ORDER BY label bound to the new column
(Parent & dj.Top(2)) & dj.Top(None, offset=1) and .to_dicts(limit=5, offset=1) rows from outside the first Top
bool(Parent & dj.Top(None, order_by="parent_id", offset=3)) True for an empty result
dj.Diagram(schema).restrict(Parent & top_parent) every row of Parent and its descendants

The fix

The rule is the one #1650 applies to a restricted left-join operand: a limited operand is wrapped in a subquery, so its ORDER BY/LIMIT applies to that operand on its own.

  • join(): an operand with _top, on either side, is a subquery. This covers *, aggr (inner and left), and extend.
  • make_condition: a limited restricting expression is a subquery: IN (SELECT a FROM (SELECT … ORDER BY … LIMIT n) AS t). This is the usual MySQL workaround for 1235, and it applies the limit before the projection to the common attributes. That projection matters on PostgreSQL too, for example when the order is on a computed attribute.
  • dj.U & expr and dj.U.aggr(group): a limited operand is a subquery, so the limit applies before DISTINCT/GROUP BY.
  • Union.make_sql: sorting_clauses() moves out of the alias name and follows it. The branch for operands with secondary attributes is wrapped the same way when the union is limited.
  • proj(): adds a subquery only when the ORDER BY names an attribute that the projection redefines (a rename or computation into that name) or a computed/renamed attribute that it drops. Dropping a plain table column needs no subquery, because ORDER BY on an unselected column is valid. So fetch(attr, order_by=..., limit=...) keeps its ordering at the outer level.
  • Top.merge: the second Top skips other.offset rows of the first Top's window and takes up to other.limit, never past the first Top's limit. Previously min(limit1, limit2) with added offsets could reach past it.
  • QueryExpression.__bool__: uses a subquery when _top is set, as __len__ already does.
  • Diagram.restrict(): builds its seed with _seed_restriction(), as cascade and trace do. A dj.Top seed, or a seed restricted by another query, becomes the primary keys it selects, and % in seed conditions is no longer escaped twice when the conditions are re-applied.
  • Heading.make_subquery_heading(): keeps table_info, so a rename after a subquery quotes for the backend. Without it, PostgreSQL got backticks. This is the same fix fix(heading): keep table_info in set_primary_key so a later rename quotes for the backend #1643 makes in set_primary_key, and it was needed once proj() started wrapping.

A - dj.Top() stays unsupported, as discussed in #1656. A negated or nested Top (-, dj.Not, a list, an AndList, or populate()/progress() restrictions) now raises:

dj.Top must be applied on its own, as expression & dj.Top(...). It cannot be negated (with - or dj.Not) or combined with other restrictions in a list, an AndList, or the restrictions passed to populate() or progress(). For the rows after the first n, use dj.Top(None, order_by=..., offset=n).

TestDjTop::test_top_errors previously pinned the old "Invalid restriction type Top(...)" text and is updated.

Behavior changes (for the v2.3.7 notes)

All of these bring results in line with what the queries say:

  • Queries in the table above return the limited rows instead of the full set.
  • dj.U(attrs) & (expr & dj.Top(n, ...)) limits expr first and then takes distinct values. The previous "first n distinct values" is written (dj.U(attrs) & expr) & dj.Top(n, ...).
  • Chained Tops and to_dicts(limit=…, offset=…) on a limited expression no longer return rows past the first limit.
  • Diagram.restrict() seeds the same way as cascade.

Tests

  • tests/integration/test_top_in_queries.py: 49 cases × MySQL and PostgreSQL.
    • Joins: the limit on either side, on both sides, with a condition, the offset form, and a projected operand.
    • Left joins: extend and default aggr, with the limit on either side.
    • Aggregation: inner aggr and dj.U.aggr, with both Top and the offset form.
    • Restriction and anti-restriction by a limited expression: Top, Top(2), the offset form, the order on a computed attribute, and a limited union.
    • dj.U restriction: the order on a dropped attribute and on a kept one.
    • Union: restricted by Top, and of limited operands, with and without secondary attributes.
    • proj() of a limited expression: keys() of a limited aggregate, a dropped computed ordering attribute, an ordering attribute recomputed or swapped by a rename, and a dropped table column, which needs no subquery.
    • Chained limits, bool() and Diagram.restrict().
    • The negated/nested Top error: five forms.
    • Controls: no subquery for unlimited joins, restrictions, dj.U & and dj.U.aggr.
  • tests/unit/test_condition.py: Top.merge windows.
  • Each change is tested: reverting any one of the 13 source changes on its own fails at least one test. That covers both backends wherever the bug shows on both; the table_info change shows only on PostgreSQL.
  • Full suite locally: 1323 passed, 14 skipped.

Interaction with other PRs

This branch merges cleanly with #1655, #1643, #1653 and #1654 (checked with git merge-tree). After #1655 merges, its or other._top in the left-join condition is redundant with the general rule here, but harmless.

Not changed here

These turned up in review. They are not about dj.Top and predate this PR:

…uery

An expression restricted by dj.Top keeps its ORDER BY/LIMIT in _top, and
several operations ignored it or broke on it:

- join() joined a limited operand directly, on either side, so the limit was
  dropped from *, aggr, and extend. A limited operand is now a subquery.
- Restriction by a limited expression put the LIMIT inside IN (...), which
  MySQL rejects (error 1235). The expression is now a subquery there.
- dj.U.aggr ignored the group's limit, and dj.U & expr applied DISTINCT
  before the limit. Both now apply the limit first.
- Union put its ORDER BY/LIMIT inside the quoted alias name (a syntax error),
  and ignored it for operands with secondary attributes.
- proj() kept the limit while dropping or redefining an attribute its
  ORDER BY names, so keys() and similar calls failed or selected other rows.
  It now adds a subquery in exactly those cases.
- Top.merge let a second Top with an offset reach past the first Top's limit.
- bool() ignored the limit.
- Diagram.restrict() ignored a dj.Top on its seed; it now builds the seed as
  cascade and trace do.
- A subquery heading dropped table_info, so a later rename quoted with
  backticks on PostgreSQL.

dj.Top still cannot be negated or nested in a list or AndList; the error now
says so and names the complement, dj.Top(None, order_by=..., offset=n).

Fixes #1656.
@dimitri-yatsenko dimitri-yatsenko added this to the v2.3.7 milestone Nov 6, 2026
@dimitri-yatsenko dimitri-yatsenko added the bug Indicates an unexpected problem or unintended behavior label Nov 6, 2026

@MilagrosMarin MilagrosMarin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dimitri-yatsenko, this is a careful fix, and the table of reproductions made it easy to check. I read each change in context:

✅ join() / make_condition / dj.U: a limited operand becomes a subquery, and the wrapped expression carries no _top, so the limit can't leak outward
✅ Top.merge: checked the window arithmetic, including the offset-only placeholder limit and the remaining == 0 case
✅ proj(): subquery only when the ORDER BY names a redefined or dropped computed attribute; KEY ordering after a key rename still resolves on both backends
✅ make_subquery_heading keeping table_info: it doesn't trigger extra lookups (table_status is only read for real tables), and select() already copies table_info the same way

A few notes, none blocking:

  • Docs follow-up: datajoint-docs src/reference/specs/query-algebra.md §12.5 still says the merged limit is "minimum of both limits". The examples there still hold, but the rule should read "the second Top's window within the first". The new dj.U & (expr & dj.Top(...)) meaning and the "no nested/negated Top" rule should probably go in §12 and reference/operators.md too. Happy to open that docs PR.
  • Diagram.restrict(): besides dj.Top, any seed restricted by another query is now read into keys, as cascade does. Worth a line in the v2.3.7 notes, since a very large seed becomes a large IN list.
  • Nit: proj() builds self.heading.select(...) twice (once for the check, once for the result); it could be built once and reused.

Approving. Let me know if you'd like me to take the docs update!

@dimitri-yatsenko

Copy link
Copy Markdown
Member Author

Thanks @MilagrosMarin.

  • Docs: yes, please take the docs update. The bullet and table rows in §12.5 of query-algebra.md need the new rule. The second Top is applied within the first one's window, so the merged limit is min(first.limit - second.offset, second.limit), never below 0, or just second.limit when the first has no limit. The current examples stay valid. An example with an offset shows where the old wording breaks: Top(5, "x") followed by Top(10, order_by=None, offset=3) gives Top(2, offset=3). The meaning of dj.U & (expr & dj.Top(...)) and the new error for a negated or nested Top belong in §12 and reference/operators.md. datajoint-docs#307 edits the same file in other sections.
  • Diagram.restrict(): agreed, a seed restricted by another query is now read into keys. That goes in the v2.3.7 release notes.
  • heading.select built twice in proj(): confirmed. I'm leaving it for a follow-up cleanup, together with the or other._top that fix: aggr, extend and union keep unmatched rows when the right operand is restricted #1655 adds to the left-join condition, which this PR makes redundant.

I also added #1666 to the issues this closes; it reports the same bug as #1656.

GitHub Actions has no run for this branch (no workflows ran between Oct 31 and Nov 9). Merged onto current master with #1655 and the other approved PRs, the full suite passes locally: 1362 tests on MySQL 8.0 and 8.4, including the tests shared with PostgreSQL, with all of test_top_in_queries passing on both backends. Merging.

@dimitri-yatsenko
dimitri-yatsenko merged commit fa62909 into master Nov 10, 2026
1 check passed
@dimitri-yatsenko
dimitri-yatsenko deleted the fix/top-in-joins branch November 10, 2026 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Indicates an unexpected problem or unintended behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Joining a dj.Top expression silently drops its limit Joins drop dj.Top: A * (B & dj.Top(n)) and (A & dj.Top(n)) * B ignore the limit

2 participants